[#10733] fix(core): Correct DBCP2 connection pool settings to avoid cold-start connection overhead#10734
Open
yuqi1129 wants to merge 1 commit intoapache:mainfrom
Open
[#10733] fix(core): Correct DBCP2 connection pool settings to avoid cold-start connection overhead#10734yuqi1129 wants to merge 1 commit intoapache:mainfrom
yuqi1129 wants to merge 1 commit intoapache:mainfrom
Conversation
…leTimeMillis=1s - minIdle: 0 → 5 to keep warm connections ready and avoid cold-start latency after idle periods. With minIdle=0, the evictor (running every 10 min) drains the pool to zero, forcing connection re-establishment on the next request. - minEvictableIdleTimeMillis: 1000ms → Duration.ofSeconds(30).toMillis() (30 s). 1 second was effectively a no-op guard since the evictor only runs every 10 minutes, and reads as a suspicious dev/test value. Using Duration.ofSeconds(30) makes the intent explicit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
yuqi1129
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the DBCP2 connection pool configuration used by SqlSessionFactoryHelper to reduce cold-start latency after idle periods by keeping a small warm pool and avoiding overly aggressive eviction.
Changes:
- Increase
BasicDataSourceminIdlefrom0to5to prevent the pool from shrinking to zero. - Increase
minEvictableIdleTimeMillisfrom1000msto30susingDurationfor clarity.
Comment on lines
90
to
+99
| dataSource.setMaxTotal(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_MAX_CONNECTIONS)); | ||
| dataSource.setMaxIdle(5); | ||
| dataSource.setMinIdle(0); | ||
| dataSource.setMinIdle(5); | ||
| dataSource.setLogAbandoned(true); | ||
| dataSource.setRemoveAbandonedOnBorrow(true); | ||
| dataSource.setRemoveAbandonedTimeout(60); | ||
| dataSource.setTimeBetweenEvictionRunsMillis(Duration.ofMillis(10 * 60 * 1000L).toMillis()); | ||
| dataSource.setTestOnBorrow(true); | ||
| dataSource.setTestWhileIdle(true); | ||
| dataSource.setMinEvictableIdleTimeMillis(1000); | ||
| dataSource.setMinEvictableIdleTimeMillis(Duration.ofSeconds(30).toMillis()); |
There was a problem hiding this comment.
The pool settings change (minIdle/minEvictableIdleTimeMillis) isn’t currently covered by a unit assertion. Please add/extend a test (e.g., in TestSqlSession#testInit) to verify the configured BasicDataSource values so this behavior change is protected from regressions.
Contributor
Author
There was a problem hiding this comment.
Those changes do not need UTs to cover it.
Comment on lines
90
to
93
| dataSource.setMaxTotal(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_MAX_CONNECTIONS)); | ||
| dataSource.setMaxIdle(5); | ||
| dataSource.setMinIdle(0); | ||
| dataSource.setMinIdle(5); | ||
| dataSource.setLogAbandoned(true); |
There was a problem hiding this comment.
The idle-size value 5 is now duplicated between setMaxIdle(5) and setMinIdle(5). Consider defining a single constant (or deriving one from the other) to prevent future mismatches when tuning the pool.
Code Coverage Report
Files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Two hardcoded DBCP2 settings in
SqlSessionFactoryHelperare corrected:minIdle:0→5minEvictableIdleTimeMillis:1000ms (1 s) →30000ms (30 s, viaDuration.ofSeconds(30).toMillis())Why are the changes needed?
Fix: #10733
The evictor thread runs every 10 minutes (
timeBetweenEvictionRunsMillis = 10 min). WithminEvictableIdleTimeMillis = 1 s, every idle connection is eligible for eviction on each evictor run. After any quiet period ≥ 10 minutes, all connections are evicted. Combined withminIdle = 0, the pool shrinks to zero, so the next burst of requests must pay a DB connection-establishment cost (TCP handshake + auth, ~20–100 ms on MySQL/PostgreSQL) before they can execute.1000 msalso reads as an accidental dev/test value. Replacing it withDuration.ofSeconds(30).toMillis()makes the intent explicit and guards against over-aggressive eviction if the evictor interval is ever shortened.Does this PR introduce any user-facing change?
No. These are internal connection pool settings with no API or config surface change.
How was this patch tested?
./gradlew :core:test -PskipITs